-
Notifications
You must be signed in to change notification settings - Fork 7.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add API functions for zend_exception #5157
Conversation
b53bd3e
to
971ffb3
Compare
ZVAL_DEREF(prop); | ||
if (Z_TYPE_P(prop) != IS_LONG) { | ||
if (!silent) { | ||
zend_type_error("Exception %s was not a long", ZSTR_VAL(ZSTR_KNOWN(id))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might it make more sense to switch these to use typed properties, so this is enforced with the now built-in mechanism? Seems like something we can do in PHP 8...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely, it makes no sense to check types during property reading. The wrong values must not be written there at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can typed properties be subverted via reflection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is certainly PDOException here where code is a string. Same could be true for any other userland exception which just writes to $this->code. Changing code to a typed string is a serious BC break.
971ffb3
to
bdd44dd
Compare
- zend_exception_get_code - zend_exception_get_file - zend_exception_get_line - zend_exception_get_message - zend_exception_get_trace - zend_exception_get_trace_as_string. This accepts an arg for controlling whether the arguments will be included.
bdd44dd
to
0f65960
Compare
I have rebased on master and removed the Regarding using typed properties: can it be subverted via reflection? If not, are there any examples of typed properties in core already? |
ZEND_API zend_long *zend_exception_get_line(zend_object *exception, zend_bool silent); | ||
ZEND_API zend_string *zend_exception_get_message(zend_object *exception, zend_bool silent); | ||
ZEND_API HashTable *zend_exception_get_trace(zend_object *exception, zend_bool silent); | ||
ZEND_API zend_string *zend_exception_get_trace_as_string(zend_object *exception, zend_bool include_args, zend_bool silent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have an argument here and in zend_exception_get_trace
that limits the number of returned items to $n?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Limiting the stack depth seems like a good idea.
This PR doesn't change the status quo -- there was a runtime error before and it has one now. I'm wondering if we can agree to just merge it in. If we get time before 8 is released we can do the follow-up improvements:
Any opposition? |
I am +1 for merging this. |
Ran out of steam for 8.0; may revisit for 8.1. |
Adds:
zend_exception_get_code
zend_exception_get_file
zend_exception_get_line
zend_exception_get_message
zend_exception_get_trace
zend_exception_get_trace_as_string
. This accepts an arg forcontrolling whether the arguments will be included.
Also adds
zend_obj_read_property
andzend_obj_read_property_ex
. These are just likezend_read_property
/zend_read_property_ex
except they take azend_object *
instead of azval *
.I intended these to mostly be used by extensions, but with a quick look did find some places where they were useful in core too.